Skip to content

Refactor RhythmBlockPaletteBlocks: Extract reusable playNoteSequence helper to remove duplicated scheduling logic#5765

Open
Pankajyadav919 wants to merge 1 commit intosugarlabs:masterfrom
Pankajyadav919:refactor/rhythm-blocks
Open

Refactor RhythmBlockPaletteBlocks: Extract reusable playNoteSequence helper to remove duplicated scheduling logic#5765
Pankajyadav919 wants to merge 1 commit intosugarlabs:masterfrom
Pankajyadav919:refactor/rhythm-blocks

Conversation

@Pankajyadav919
Copy link
Contributor

##Summary
This PR refactors js/blocks/RhythmBlockPaletteBlocks.js by extracting duplicated setTimeout-based note scheduling logic from:
-RhythmBlock.flow
-Tuplet4Block.flow

##Problem

Both RhythmBlock and Tuplet4Block implemented nearly identical logic to:
-Loop through beat values
-Compute beat duration using bpmFactor
-Schedule notes using setTimeout
-Handle final cleanup callback
-Call tur.doWait(...)

This duplication:
-Violates DRY principles
-Makes future timing fixes harder
-Increases risk of inconsistent behavior
-Complicates testing and maintenance

##Changes
[NEW]
Add playNoteSequence(activity, beatValues, blk, turtle, bpmFactor, onComplete) helper function to centralize note scheduling logic.

Responsibilities:
-Calculate beat duration
-Schedule notes using setTimeout
-Handle final callback
-Perform tur.doWait timing synchronization

@github-actions
Copy link
Contributor

✅ All Jest tests passed! This PR is ready to merge.

@github-actions
Copy link
Contributor

✅ All Jest tests passed! This PR is ready to merge.

@github-actions
Copy link
Contributor

✅ All Jest tests passed! This PR is ready to merge.

Copy link
Contributor

@vanshika2720 vanshika2720 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Pankajyadav919 Unrelated changes in ja.json should be removed. Please keep this PR focused .

@github-actions
Copy link
Contributor

✅ All Jest tests passed! This PR is ready to merge.

@Pankajyadav919
Copy link
Contributor Author

@vanshika2720 Thank you for the review.
You’re absolutely right — the changes in ja.json are unrelated to this refactor. I have removed those modifications to keep the PR strictly focused on refactoring the duplicated setTimeout logic in RhythmBlock and Tuplet4Block.

@vanshika2720
Copy link
Contributor

@Pankajyadav919 still unrelated changes.
Please restore the original UTF-8 characters and resubmit with a clean diff limited strictly to the scheduling change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants